feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult - #732

Good catch. This was also highlighted by Matt and I addressed it in #732 (comment)

Addressed in fcde3da

Addressed in 0288679

The isUpdate := … declarations inside the two cases above shadow the outer var isUpdate *bool, so this if isUpdate != nil always sees nil — it's dead code. The cases already set result.IsUpdate directly, so the outer var isUpdate *bool and this block can both be removed.

Thank you for this review.
Addressed in 3466969

Returning the internal *bool leaks mutable state (a caller can do *ps.IsUpdate() = false and mutate the statement) and isn't idiomatic for an optional. Prefer IsUpdate() (val bool, ok bool), or return a copy.

Thank you for pointing this out. I took the idiomatic approach with the double boolean return, see 2c8c152

Please add a test for the unset/nil case. This is my only blocking request.

Both new client tests here, and both new server tests, assert ok == true. Nothing anywhere asserts ok == false. That's the path taken by every server that doesn't implement the new field — i.e. the backward-compatibility contract — and it's currently the only untested branch of the getter.

TestPreparedStatementExecute above (line 368) already builds an ActionCreatePreparedStatementResult with no IsUpdate, so this can be as small as adding two lines there:

val, ok := prepared.IsUpdate()
s.False(ok)
s.False(val)

Worth locking down explicitly because a future refactor that switched isUpdate *bool to a plain bool would silently turn "no hint" into "is a query" — which is a real behavior change for clients that branch on this — and no existing test would catch it.

Do we want the s.False(val) check? If s.False(ok), then val is meaningless.

I addressed this in eb16088

I do not think it makes sense to have the check in val: it being False seems like an implementation detail, and not a part of the contract of the IsUpdate() method. Let me know if you disagree: I can change it to look exactly like your suggestion.

Two small things on this propagation site.

1. Untested. TestPreparedStatementLoadFromResult (client_test.go:971) is the only coverage for LoadPreparedStatementFromResult, and it doesn't assert IsUpdate. Since that test already constructs the result and checks the other two propagated fields (ParameterSchema, DatasetSchema), adding an IsUpdate assertion there would be consistent and nearly free. (Couldn't leave this as an inline comment on that line — it's outside the diff.)

2. Nit, no change required: this is the one place that stores a *bool owned by the caller, so a caller mutating result.IsUpdate after this call would change the statement's view. Prepare/PrepareSubstrait don't have this property since they point at their own unmarshaled local. Not worth a defensive copy unless you'd want it for handle too — just noting it so the asymmetry is a conscious choice.

1. I addressed this with 2 tests. In TestPreparedStatementLoadFromResult I let things as they are, and tested with the new field unset, see 8ba207b. Note that this test has the same nuance about val that I pointed out in feat(arrow/flight/sql): Add is_update field to ActionCreatePreparedStatementResult #732 (comment)

I added a new test that checks that the field is properly loaded also when set, see a334b86

2. For that one I purposefully followed the example set by handle, but I will admit I had my own reservations while doing it.

I will not change this in this PR unless you ask me to, but in your opinion is it worth a follow-up PR to have a defensive copy for both handle and isUpdate? If so I can tackle that after this one

Not a defect, but this needs a release note, because the public API impact is wider than this struct.

types.go:882 has:

type CreatePreparedStatementResult = pb.ActionCreatePreparedStatementResult

Being a type alias, the regenerated proto field lands on a public flightsql symbol too. So this PR adds an exported field to two publicly-reachable structs. Keyed composite literals are unaffected (and that's what I found in the downstream usage I spot-checked), but any downstream unkeyed literal like:

return flightsql.ActionCreatePreparedStatementResult{handle, ds, ps}, nil

will stop compiling. That's permitted under the Go 1 compatibility rules — unkeyed literals of imported struct types are explicitly not covered — and go vet's composites check flags them, so real-world exposure should be low. I don't think it should block the PR, but it's worth a line in the changelog rather than letting someone discover it at upgrade time.

The unconditional result.IsUpdate = output.IsUpdate on both the statement and Substrait branches is correct, by the way — both sides are *bool, so nil flows through and no presence check is needed. Good that the Substrait path wasn't forgotten.

The comment mentions 2 distinct public structs. For clarity, you are asking about release notes for both of them, right?

I updated the PR description with the breaking change. I looked into how release notes are generated, and it seems like at release time Github will use the PR title in the release notes, but not the description. Given that the description is the place where the breaking change is highlighted, do I need to add release notes elsewhere?

Explanation of the 2 structs

If you agree with my PR description you don't need to read this detail, I leave it for clarity in case I misunderstood your ask.

The type alias refers to flightsql.CreatePreparedStatementResult. This is the raw proto version, what goes in the wire, and I think it's there so that users of the flightsql package don't need to import the proto package directly. In this codebase, this is only used in LoadPreparedStatementFromResult()

But the line about the unkeyed literal is referring to a different struct flightsql.ActionCreatePreparedStatementResult that is unrelated to the alias. It is defined in server.go, and it is a developer-friendly type with the *arrow.Schema objects decoded from wire format. It is used in quite a few places in the codebase.

Both of them fall under the situation of a public struct where unkeyed literals will break, so I put both in the PR description.

Thank you for pointing out the compatibility rules, for reference I found them at https://go.dev/doc/go1compat:

For the addition of features in later point releases, it may be necessary to add fields to exported structs in the API. Code that uses unkeyed struct literals (such as pkg.T{3, "x"}) to create values of these types would fail to compile after such a change. However, code that uses keyed literals (pkg.T{A: 3, B: "x"}) will continue to compile after such a change. We will update such data structures in a way that allows keyed struct literals to remain compatible, although unkeyed literals may fail to compile.